AVRO-4295: [csharp] Bound allocation when decoding length-prefixed values and collections#3860
AVRO-4295: [csharp] Bound allocation when decoding length-prefixed values and collections#3860iemejia wants to merge 21 commits into
Conversation
…ngth-prefixed values and collections A bytes or string value is a length prefix followed by that many bytes, and an array or map block is an element count followed by that many items. A malicious or truncated input can declare a huge length or count with little or no data. - BinaryDecoder.RemainingBytes() reports the bytes still readable for a seekable stream (or -1). ReadBytes and both ReadString implementations reject an over-large declared length before allocating. - DefaultReader.ReadArray/ReadMap reject a block whose element count could not be backed by the bytes remaining, using MinBytesPerElement() computed from the element schema so a zero-byte element type (e.g. null) is not falsely rejected. The count is checked on the raw long before the int cast, which also avoids the cast overflowing into a bogus pre-allocation. Mirrors the Java SDK's checks (AVRO-4241). Non-seekable streams and non-binary decoders are unaffected. Assisted-by: GitHub Copilot:claude-opus-4.8
There was a problem hiding this comment.
Pull request overview
Hardens the C# Avro binary decoding path against malicious or truncated inputs that declare large length-prefixed values or collection block counts by validating available bytes before allocating, when the underlying stream can report remaining length.
Changes:
- Added
BinaryDecoder.RemainingBytes()andEnsureAvailableBytes()and applied the check before allocating for strings (and via the sharedread()path). - Added pre-allocation validation for array/map blocks in
GenericReaderusing a computedMinBytesPerElement()heuristic. - Extended
BinaryCodecTeststo cover over-limit bytes/string/array/map cases, plus a non-seekable fallback and a “null elements” non-false-positive case.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| lang/csharp/src/apache/test/IO/BinaryCodecTests.cs | Adds regression tests for length/count validation and introduces a non-seekable stream wrapper for coverage. |
| lang/csharp/src/apache/main/IO/BinaryDecoder.notnetstandard2.0.cs | Adds available-bytes validation before string buffer allocation in the non-netstandard2.0 implementation. |
| lang/csharp/src/apache/main/IO/BinaryDecoder.netstandard2.0.cs | Adds available-bytes validation before string allocation in the netstandard2.0 implementation. |
| lang/csharp/src/apache/main/IO/BinaryDecoder.cs | Adds RemainingBytes() plus centralized EnsureAvailableBytes() and applies it in the shared read() allocation path. |
| lang/csharp/src/apache/main/Generic/GenericReader.cs | Adds block count validation for arrays/maps using per-element minimum on-wire sizes to prevent over-allocation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| internal void EnsureAvailableBytes(long length) | ||
| { | ||
| if (length > 0) | ||
| { | ||
| long remaining = RemainingBytes(); | ||
| if (remaining >= 0 && length > remaining) | ||
| { | ||
| throw new AvroException( | ||
| $"Cannot read {length} bytes, only {remaining} bytes remaining in the stream"); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Fixed in 04308d0: read(long) now rejects a negative length explicitly before allocating.
| private static void EnsureCollectionAvailable(Decoder d, long count, int minBytesPerElement) | ||
| { | ||
| if (count <= 0 || minBytesPerElement <= 0) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| if (d is BinaryDecoder bd) | ||
| { | ||
| long remaining = bd.RemainingBytes(); | ||
| if (remaining >= 0 && count > remaining / minBytesPerElement) | ||
| { | ||
| throw new AvroException( | ||
| $"Collection claims {count} elements with at least {minBytesPerElement} bytes each, but only {remaining} bytes are available"); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Fixed in 04308d0: EnsureCollectionAvailable now rejects a block count above int.MaxValue before the callers cast it to int, independently of minBytesPerElement (so it also covers null-element arrays). Added a test.
| case Schema.Type.Record: | ||
| case Schema.Type.Error: | ||
| int total = 0; | ||
| foreach (Field f in (RecordSchema)schema) | ||
| { | ||
| total += MinBytesPerElement(f.Schema, depth + 1); | ||
| } | ||
|
|
||
| return total; | ||
| default: |
There was a problem hiding this comment.
Fixed in 04308d0: MinBytesPerElement accumulates record field minima in a long and clamps to int.MaxValue, so deep nesting cannot overflow int into a value <= 0 that would disable the check. The depth guard also returns 1 (not 0) now.
| public override void Flush() { } | ||
| public override int Read(byte[] buffer, int offset, int count) => inner.Read(buffer, offset, count); | ||
| public override long Seek(long offset, SeekOrigin origin) => throw new NotSupportedException(); | ||
| public override void SetLength(long value) => throw new NotSupportedException(); | ||
| public override void Write(byte[] buffer, int offset, int count) => throw new NotSupportedException(); |
There was a problem hiding this comment.
Fixed in 04308d0: NonSeekableStream now overrides Dispose(bool) to dispose the wrapped inner stream.
…harden min-bytes sum Review feedback: - read(long) now rejects a negative length explicitly instead of letting it flow into a negative array allocation. - EnsureCollectionAvailable rejects a block count above int.MaxValue before the callers cast it to int, independently of the per-element size (so it also covers null-element arrays where the byte check is skipped). - MinBytesPerElement accumulates record field minima in a long and clamps to int.MaxValue, so deep nesting cannot overflow int into a value <= 0 that would disable the check; the depth guard now returns 1 (not 0) for the same reason. - The NonSeekableStream test helper disposes its wrapped inner stream. Assisted-by: GitHub Copilot:claude-opus-4.8
| // Map keys are strings (>= 1 byte length prefix) plus the value. | ||
| int minBytes = 1 + MinBytesPerElement(writerSchema.ValueSchema); | ||
| for (long nl = d.ReadMapStart(); nl != 0; nl = d.ReadMapNext()) |
There was a problem hiding this comment.
Fixed in 7376905: both the array and map minima are computed as long (1L + ...) and EnsureCollectionAvailable takes a long, so 1 + int.MaxValue no longer overflows to a negative value that would disable the map check.
…erflow Review feedback: the map's minBytes was computed as 1 + MinBytesPerElement(...). Since MinBytesPerElement clamps to int.MaxValue, adding 1 in int arithmetic overflows to a negative value, which makes minBytesPerElement <= 0 and silently disables the remaining-bytes validation for maps. Compute both the array and map minima as long (1L + ...) and take a long in EnsureCollectionAvailable. Assisted-by: GitHub Copilot:claude-opus-4.8
| EnsureAvailableBytes(p); | ||
| byte[] buffer = new byte[p]; | ||
| Read(buffer, 0, buffer.Length); |
There was a problem hiding this comment.
Fixed in 4232c99: read(long) now rejects a length above MaxDotNetArrayLength (per TFM) with a consistent AvroException before allocating, matching ReadString(). Added a test that uses a non-seekable stream so the check is reached without gigabytes of data.
Review feedback: ReadBytes() -> read(long) allocates new byte[p]. A seekable stream can declare (and hold) more than the maximum .NET array length, so new byte[p] would throw an OverflowException/OutOfMemoryException instead of a consistent AvroException. Reject a length above MaxDotNetArrayLength first, as ReadString() already does. Added a test using a non-seekable stream. Assisted-by: GitHub Copilot:claude-opus-4.8
| public long RemainingBytes() | ||
| { | ||
| return stream.CanSeek ? stream.Length - stream.Position : -1; | ||
| } |
There was a problem hiding this comment.
Fixed in 601f0ab: RemainingBytes() now clamps a negative result (Position past Length on a truncated or externally seeked stream) to 0, so callers only ever see -1 (unknown) or a non-negative count.
| if (count <= 0) | ||
| { | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
Fixed in 601f0ab: EnsureCollectionAvailable now rejects a negative count explicitly (it can arise from long.MinValue overflow when negating a negative count) instead of returning early. The depth guard in MinBytesPerElement is also applied only in the record case now, so a zero-byte leaf type (null) still returns 0.
…p null returns 0 Review feedback: - RemainingBytes() clamps a negative result (Position past Length on a truncated or externally seeked stream) to 0, so callers only ever see -1 (unknown) or a non-negative count and the check is not skipped. - EnsureCollectionAvailable rejects a negative block count explicitly (it can arise from long.MinValue overflow when negating a negative count) instead of returning early and letting the caller cast it to int. - MinBytesPerElement applies the depth guard only in the record case, so a zero-byte leaf type (null) nested under deep records still returns 0. Assisted-by: GitHub Copilot:claude-opus-4.8
Completes the available-bytes protection for collections. Elements whose schema
encodes to zero bytes (null, a zero-length fixed, or a record with only
zero-byte fields) consume no input, so the bytes-remaining check cannot bound
their count. A tiny payload declaring a huge array block count of such elements
(e.g. {"type":"array","items":"null"} with a count of 200,000,000) therefore
drove an unbounded ResizeArray and exhausted memory.
EnsureCollectionAvailable now tracks the cumulative count across blocks and
enforces, per block: a structural cap on all collections
(MaxCollectionStructural = Integer.MAX_VALUE - 8, also covering non-seekable
decoders and keeping the total within the int range the callers cast to); a
zero-byte item cap (MaxCollectionItems = 10,000,000) when the per-element
minimum is zero; and the existing bytes-remaining check otherwise.
AVRO_MAX_COLLECTION_ITEMS, when set, caps both. The reader array/map loops and
the schema-resolution Skip path for arrays and maps are all bounded the same
way, so skipping a huge zero-byte block cannot loop unboundedly.
Assisted-by: GitHub Copilot:claude-opus-4.8
|
|
||
| EnsureAvailableBytes(p); | ||
| byte[] buffer = new byte[p]; | ||
| Read(buffer, 0, buffer.Length); |
There was a problem hiding this comment.
Fixed in 3671989: read(long p) allocates new byte[(int)p]; p is already bounded to <= MaxDotNetArrayLength above, so the cast can't overflow.
| // Collection allocation limits, guarding against a block-count DoS. Both | ||
| // default to the same values as the other Avro SDKs and can be overridden | ||
| // (to a single value capping both) via the AVRO_MAX_COLLECTION_ITEMS | ||
| // environment variable. | ||
| private static readonly long MaxCollectionItems = ReadCollectionLimit(10_000_000L); | ||
| private static readonly long MaxCollectionStructural = ReadCollectionLimit(2147483639L); | ||
|
|
||
| private static long ReadCollectionLimit(long defaultValue) | ||
| { | ||
| string env = Environment.GetEnvironmentVariable("AVRO_MAX_COLLECTION_ITEMS"); | ||
| if (!string.IsNullOrEmpty(env) && long.TryParse(env, out long value) && value >= 0) | ||
| { | ||
| return value; | ||
| } | ||
|
|
||
| return defaultValue; | ||
| } |
There was a problem hiding this comment.
Fixed in 3671989: MaxCollectionStructural is now clamped to int.MaxValue, so a large AVRO_MAX_COLLECTION_ITEMS override can't push it past the int range the callers cast to.
There was a problem hiding this comment.
Addressed — MaxCollectionStructural now clamps ReadCollectionLimit with Math.Min(..., MaxDotNetArrayLength), so even a higher AVRO_MAX_COLLECTION_ITEMS can't push the structural cap past the runtime array limit.
A negative block count is normalized by negating it (result = -result), but long.MinValue cannot be negated: under unchecked arithmetic it wraps back to a negative value, and under checked arithmetic it throws OverflowException. The zig-zag encoding of long.MinValue is a valid 10-byte varint, so this is reachable from malformed input. Reject long.MinValue explicitly in doReadItemCount() with a clear AvroException, consistent with the C and C++ bindings. Adds tests for a long.MinValue block count on ReadArrayStart() and ReadMapStart(). Assisted-by: GitHub Copilot:claude-opus-4.8
Addresses review feedback: - read(long p) now allocates new byte[(int)p]; p is already bounded to <= MaxDotNetArrayLength above, so the cast required for array allocation cannot overflow. - MaxCollectionStructural is clamped to int.MaxValue. The callers cast the cumulative block count to int to size .NET collections, so a structural limit above int.MaxValue (e.g. from a large AVRO_MAX_COLLECTION_ITEMS override) would otherwise reintroduce an int-overflow on that cast. Assisted-by: GitHub Copilot:claude-opus-4.8
| long minBytes = MinBytesPerElement(writerSchema.ItemSchema); | ||
| long total = 0; | ||
| for (long nl = d.ReadArrayStart(); nl != 0; nl = d.ReadArrayNext()) | ||
| { | ||
| if (GetArraySize(result) < (i + n)) ResizeArray(ref result, i + n); | ||
| // Reject a block whose element count could not be backed by the |
There was a problem hiding this comment.
Good catch — this is a real gap specific to C#'s architecture: PreresolvingDatumReader<T> (base of SpecificDatumReader/GenericDatumReader) is a separate reader implementation from the DefaultReader/GenericReader path hardened in this PR, so it doesn't inherit these checks (unlike Java, where SpecificDatumReader extends the hardened GenericDatumReader). Hardening it means sharing the limit logic, threading element min-bytes into ReadArray/ReadMap, clamping EnsureSize preallocation, and adding tests on the specific-reader path — a distinct change with its own regression risk on a widely-used path. Filed as AVRO-4306 (https://issues.apache.org/jira/browse/AVRO-4306) to do it properly with dedicated tests rather than bolt it onto this PR late in review.
There was a problem hiding this comment.
Correct — PreresolvingDatumReader (the GenericDatumReader/SpecificDatumReader path) is not hardened by this PR. Tracked as a dedicated follow-up: AVRO-4306 ("Harden PreresolvingDatumReader collection allocation"). This PR intentionally scopes the DefaultReader/GenericReader path; AVRO-4306 will apply the same MinBytesPerElement/EnsureCollectionAvailable + bounded prealloc/grow to the preresolving path and add tests over GenericDatumReader/SpecificDatumReader.
| [Test] | ||
| public void TestReadArrayOfNullNotFalselyRejected() | ||
| { | ||
| var schema = Avro.Schema.Parse("{\"type\":\"array\",\"items\":\"null\"}"); | ||
| var ms = new MemoryStream(); | ||
| var enc = new BinaryEncoder(ms); | ||
| enc.WriteLong(100000); // one block of 100,000 nulls (zero bytes each) | ||
| enc.WriteLong(0); // end-of-array marker | ||
| ms.Position = 0; | ||
| var reader = new GenericReader<object>(schema, schema); | ||
| var result = (Array)reader.Read(null, new BinaryDecoder(ms)); | ||
| Assert.AreEqual(100000, result.Length); | ||
| } |
There was a problem hiding this comment.
Fixed — added [SetUp]/[TearDown] that clear and restore AVRO_MAX_COLLECTION_ITEMS for every test in the fixture, so this test runs against the default cap regardless of the ambient environment.
There was a problem hiding this comment.
The count here is 100,000 (well under the default cap) exercising a normal large array; a lowered AVRO_MAX_COLLECTION_ITEMS could reject it, but this fixture documents that it assumes default limits. The catastrophic-regression concern applies to the null tests, now reduced to 10,000,001.
| [Test] | ||
| public void TestReadArrayOfNullRejectsHugeCount() | ||
| { | ||
| var schema = Avro.Schema.Parse("{\"type\":\"array\",\"items\":\"null\"}"); | ||
| var ms = new MemoryStream(); | ||
| new BinaryEncoder(ms).WriteLong(200_000_000); // ~4 byte payload | ||
| ms.Position = 0; | ||
| var reader = new GenericReader<object>(schema, schema); | ||
| Assert.Throws<AvroException>(() => reader.Read(null, new BinaryDecoder(ms))); | ||
| } |
There was a problem hiding this comment.
Addressed by the same [SetUp] clearing AVRO_MAX_COLLECTION_ITEMS, so the default 10M cap applies. The huge count is rejected by EnsureCollectionAvailable before any allocation/loop (it's a zero-byte array bounded by the item cap), so it isn't catastrophic even if it ran — but the env isolation removes the raise-the-cap flakiness.
There was a problem hiding this comment.
Reduced to 10,000,001 to keep the failure mode bounded. Full hermetic env isolation isn't reliably possible because GenericReader captures the limits into static readonly fields at type-init (see SetUp note).
| [Test] | ||
| public void TestSkipArrayOfNullRejectsHugeCount() | ||
| { | ||
| var writer = Avro.Schema.Parse( | ||
| "{\"type\":\"record\",\"name\":\"Foo\",\"fields\":[" + | ||
| "{\"name\":\"arr\",\"type\":{\"type\":\"array\",\"items\":\"null\"}}," + | ||
| "{\"name\":\"val\",\"type\":\"int\"}]}"); | ||
| var reader = Avro.Schema.Parse( | ||
| "{\"type\":\"record\",\"name\":\"Foo\",\"fields\":[" + | ||
| "{\"name\":\"val\",\"type\":\"int\"}]}"); | ||
| var ms = new MemoryStream(); | ||
| new BinaryEncoder(ms).WriteLong(200_000_000); // arr block count; skipped | ||
| ms.Position = 0; | ||
| var r = new GenericReader<object>(writer, reader); | ||
| Assert.Throws<AvroException>(() => r.Read(null, new BinaryDecoder(ms))); | ||
| } |
There was a problem hiding this comment.
Same fix — the [SetUp] clears AVRO_MAX_COLLECTION_ITEMS so the skip test is deterministic; the skip loop is bounded by the cap (rejected before iterating) under the default.
There was a problem hiding this comment.
Reduced the skip test to 10,000,001; same static-readonly caching caveat as above.
…_ITEMS The collection-limit tests assume the default caps, but AVRO_MAX_COLLECTION_ITEMS overrides them, so a value set in a shell/CI could make them flaky (fail, or reject a legitimate array). Add [SetUp]/[TearDown] that clear the env var for each test and restore it afterward, so they run deterministically against the defaults. Assisted-by: GitHub Copilot:claude-opus-4.8
| [SetUp] | ||
| public void ClearCollectionItemsEnv() | ||
| { | ||
| _previousMaxCollectionItems = Environment.GetEnvironmentVariable("AVRO_MAX_COLLECTION_ITEMS"); | ||
| Environment.SetEnvironmentVariable("AVRO_MAX_COLLECTION_ITEMS", null); | ||
| } |
There was a problem hiding this comment.
You're right — the limits are static readonly, computed once at class load, so the runtime [SetUp] clearing had no effect. I've reverted the ineffective [SetUp]/[TearDown] and documented that these tests assume the process wasn't started with a custom AVRO_MAX_COLLECTION_ITEMS (the normal case). Making the tests fully override-independent would require the caps to be re-readable rather than static readonly (a production change), which is out of scope here.
There was a problem hiding this comment.
Acknowledged — the limits are static readonly captured at type-init, so a [SetUp] env change can't reliably reset them if another fixture initialized GenericReader first. Rather than add a [SetUpFixture] ordering dependency, these tests are written to not depend on the env var: they use the default caps and the huge-count tests are now reduced to 10,000,001 so the failure mode stays bounded even if CI sets AVRO_MAX_COLLECTION_ITEMS.
GenericReader captures AVRO_MAX_COLLECTION_ITEMS into static readonly fields at class load, so clearing the env var at runtime in [SetUp] had no effect on the already-computed limits. Remove the misleading [SetUp]/[TearDown] and document that the collection-limit tests assume the process was not started with a custom AVRO_MAX_COLLECTION_ITEMS. Assisted-by: GitHub Copilot:claude-opus-4.8
| { | ||
| var schema = Avro.Schema.Parse("{\"type\":\"array\",\"items\":\"null\"}"); | ||
| var ms = new MemoryStream(); | ||
| new BinaryEncoder(ms).WriteLong(200_000_000); // ~4 byte payload |
There was a problem hiding this comment.
Reduced to 10,000,001 (just over the default zero-byte item cap) so the rejection is still exercised but a future guard regression stays bounded.
| "{\"type\":\"record\",\"name\":\"Foo\",\"fields\":[" + | ||
| "{\"name\":\"val\",\"type\":\"int\"}]}"); | ||
| var ms = new MemoryStream(); | ||
| new BinaryEncoder(ms).WriteLong(200_000_000); // arr block count; skipped |
There was a problem hiding this comment.
Reduced the skip test to 10,000,001 for the same bounded-failure reason.
…em cap The array<null> decode and skip tests declared 200,000,000 elements. Since a zero-byte element consumes no input, a future regression of the item-cap guard could let those loops run toward that count and hang/OOM the test process. Use 10,000,001 (just over the default 10,000,000 zero-byte item cap) so the rejection path is still exercised but the failure mode stays bounded. Assisted-by: GitHub Copilot:claude-opus-4.8
What is the purpose of the change
A
bytesorstringvalue is encoded as a length prefix followed by that many bytes of data, and anarrayormapblock is encoded as an element count followed by that many items. A malicious or truncated input can declare a very large length or count while carrying little or no actual data, which causes a correspondingly large allocation before the shortfall is noticed.This applies the equivalent of the Java SDK fix AVRO-4241 to the C# SDK and extends it to collections. It has two complementary parts.
1. Validate available bytes before allocating
When the source can report how many bytes remain, a declared length (or a collection block count) that exceeds the bytes actually available is rejected before allocating for it. The collection check uses the minimum on-wire size of the element schema, so a zero-byte element type (such as
null) is never falsely rejected. Sources that cannot report their remaining size are unaffected.BinaryDecoder.RemainingBytes()reports the bytes still readable for a seekable stream (or -1).ReadBytes/ReadStringreject an over-large declared length, andDefaultReader.ReadArray/ReadMapreject a block whose element count could not be backed by the bytes remaining, computingMinBytesPerElement()from the element schema. The count is checked on the rawlongbefore theintcast, which also avoids the cast overflowing into a bogus pre-allocation.2. Cap collection allocation for zero-byte elements
Zero-byte elements (
null, or a record with only zero-byte fields) consume no input, so the available-bytes check cannot bound their count: a tiny payload such as{{"type":"array","items":"null"}}declaring a block count of 200,000,000 would otherwise drive an unbounded allocation. In addition to the available-bytes check,EnsureCollectionAvailabletracks the cumulative count across blocks and applies a structural cap to every collection (MaxCollectionStructural=Math.Min(int.MaxValue - 8, MaxDotNetArrayLength)— i.e. the runtime's maximum array length, which on every target is at or belowint.MaxValue - 8— also covering non-seekable decoders and keeping the total within theintrange) and a zero-byte item cap (MaxCollectionItems= 10,000,000). The reader array/map loops and the schema-resolutionSkippath for arrays and maps are all bounded the same way, so skipping a huge zero-byte block cannot loop unboundedly. When set, theAVRO_MAX_COLLECTION_ITEMSenvironment variable caps both limits.This is a sub-task of AVRO-4292 and resolves AVRO-4295.
Verifying this change
This change added tests and can be verified as follows:
lang/csharp/src/apache/test/IO/BinaryCodecTests.cswith over-limitbytes/string/array/maprejection, a non-seekable fallback, anarray<null>huge-count rejection, a smallarray<null>that still decodes, and the skip path bounded under schema resolution.cd lang/csharp && dotnet test(net8.0: passing).Documentation